feat(resolve): model skills as directories instead of single files - #2139

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go:843 — ListDirectoryContents recurses into subdirectories with maxDirDepth=10, maxDirAPIcalls=100, and maxDirFiles=1000 shared counters. While these counters bound total work within a single invocation, up to 100 sequential GitHub API calls can occur within a single ListDirectoryContents call, and these are not counted against the resolver's maxResources limit. A skill directory with many subdirectories could cause significant latency without hitting the resource limit.
  Remediation: Consider counting each ListDirectoryContents invocation against maxResources, or passing a budget callback so the forge method can coordinate with the resolver's resource counter.

• [stale-doc] docs/guides/user/customizing-agents.md — Multiple examples show skills as single .md files (skills/my-custom-validation.md, skills/my-skill.md, skills/my-custom-linting.md) rather than directories containing SKILL.md. The companion doc customizing-with-skills.md correctly describes the directory model. Users following customizing-agents.md would create incorrect skill structures. This is pre-existing staleness exposed by the directory model change, not introduced by this PR.
  Remediation: Update all skill path examples in customizing-agents.md to show the directory structure (e.g., skills/my-skill/SKILL.md).

Low

• [scope-expansion] PR title suggests "model skills as directories" but also implements lock file functionality (Phase 3). The lock file changes (Type/Files fields in DependencyEntry) are tightly coupled to the directory model and cannot easily be separated.

• [forge-client-instantiation] internal/cli/lock.go, internal/cli/run.go — Both files use resolveToken() for GitHub token resolution when HasURLSkills() is true. Verify that the error message mentions all authentication options including gh auth login for users who don't set environment variables.

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Intentional design: if there is no SKILL.md, there are no dependencies to declare. Validation of skill directory contents belongs at a different layer.

• [edge-case] internal/forge/github/github.go:880 — In listDirContents, when path is empty (repository root), the relative path computation uses relPath = e.Path. Behavior depends on the GitHub API returning paths without leading slashes, which is a stable API contract.

• [test-adequacy] internal/cli/lock_test.go — TestRunLock_SkillDirectoryRoundTrip now provides round-trip coverage (generate lock via runLock, then resolveFromLock). Prior finding addressed.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [path-traversal] internal/fetch/cache.go — CacheGetDir uses filepath.Walk to traverse the tree/ directory during integrity re-verification. filepath.Walk follows symlinks by default. An attacker with write access to the cache directory could plant a symlink pointing outside the cache root. Practical exploitability is low due to multiple defense layers (validateCachePath, atomicWrite, CachePutDir path traversal check, and integrity hash verification).

• [architectural-coherence] internal/resolve/resolve.go:77 — ResolveHarness godoc discusses transitive trust but does not reference ADR-0038 where the security model is documented. Adding a cross-reference would help maintainers understand the trust model.

Info

• [missing-authorization] No linked issue for a non-trivial 23-file PR. The change aligns with accepted ADR-0038, providing architectural rationale.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. Matches the established pattern in ParseIntegrityHash. A literal # in a URL path is essentially impossible in practice.

• [open-redirect-via-relative-url] internal/resolve/relurl.go:11 — ResolveRelativeURL passes through absolute URLs without validation. Defense is effective via downstream allowlist check in resolveURL.

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go:843 — ListDirectoryContents recurses into subdirectories with maxDirDepth=10 and maxDirAPIcalls=100 shared counter. While the API call counter bounds total calls, up to 100 sequential GitHub API calls can occur within a single ListDirectoryContents invocation, and these are not counted against the resolver's maxResources limit. A malicious or excessively large skill directory could cause significant latency without hitting the resource limit.
  Remediation: Consider counting each ListDirectoryContents invocation against maxResources, or adding a maxFiles limit to bound the total number of file entries returned.

Low

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality (directory model, forge API methods, lock file extensions). Per COMMITS.md, fix is for user-visible bug fixes. This should be feat(resolve): — GoReleaser uses the prefix to categorize release notes.

• [forge-client-instantiation] internal/cli/lock.go, internal/cli/run.go — Both files directly check os.Getenv for GH_TOKEN/GITHUB_TOKEN instead of using the established resolveToken() pattern found in admin.go. Users who authenticate via gh auth login (no env var) will get an error when running fullsend lock or fullsend run with skill URLs. Error message also omits the gh auth login fallback mentioned by the standard pattern.

• [path-traversal] internal/fetch/cache.go:281 — CacheGetDir uses filepath.Walk to traverse the tree/ directory during integrity re-verification. filepath.Walk follows symlinks by default. An attacker with write access to the cache directory could plant a symlink pointing outside the cache root. Practical exploitability is low due to multiple defense layers (validateCachePath, atomicWrite, CachePutDir path traversal check).

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Intentional design: if there is no SKILL.md, there are no dependencies to declare. Validation of skill directory contents belongs at a different layer.

• [edge-case] internal/forge/github/github.go:880 — In listDirContents, when path is empty (repository root), the relative path computation uses relPath = e.Path. Behavior depends on the GitHub API returning paths without leading slashes, which is a stable API contract.

• [missing-migration-path] docs/ADRs/0038-universal-harness-access.md — The ADR updates skills from single files to directories but does not document a migration path for existing harnesses with single-file skill URLs. The code now rejects non-forge skill URLs at validation time with a clear error, and lock file backward compatibility is handled via empty Type defaulting to "file".

• [scope-expansion] PR title suggests "model skills as directories" but also implements lock file functionality (Phase 3). However, the lock file changes are tightly coupled to the directory model (Type/Files fields) and cannot easily be separated.

• [test-adequacy] internal/cli/lock_test.go — No test verifying the round-trip: generating a lock file with a directory skill via runLock and then using resolveFromLock to resolve from that lock file in a single test. Separate tests exist for each direction.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Essentially impossible in practice since URLs are programmatically constructed.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [stale-harness-example] docs/ADRs/0024-harness-definitions.md:233 — The harness schema example shows skills as individual SKILL.md files rather than directories, predating the directory model introduced by this PR.

Info

• [missing-authorization] No linked issue for a non-trivial 22-file PR. The change aligns with accepted ADR-0038, providing architectural rationale.

• [stale-reference-to-skill-format] docs/runtimes.md:12 — The security matrix references SKILL.md as a scanned artifact. This is not misleading — SKILL.md is still the file within a skill directory that gets scanned.

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go:843 — ListDirectoryContents recurses into subdirectories with maxDirDepth=10 and maxDirAPIcalls=100 shared counter. While the API call counter bounds total calls, up to 100 sequential GitHub API calls can occur within a single ListDirectoryContents invocation, and these are not counted against the resolver's maxResources limit. A malicious or excessively large skill directory could cause significant latency without hitting the resource limit.
  Remediation: Consider counting each ListDirectoryContents invocation against maxResources, or adding a maxFiles limit to bound the total number of file entries returned.

Low

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality (directory model, forge API methods, lock file extensions). Per COMMITS.md, fix is for user-visible bug fixes. This should be feat(resolve): — GoReleaser uses the prefix to categorize release notes.

• [forge-client-instantiation] internal/cli/lock.go, internal/cli/run.go — Both files directly check os.Getenv for GH_TOKEN/GITHUB_TOKEN instead of using the established resolveToken() pattern found in admin.go. Users who authenticate via gh auth login (no env var) will get an error when running fullsend lock or fullsend run with skill URLs. Error message also omits the gh auth login fallback mentioned by the standard pattern.

• [path-traversal] internal/fetch/cache.go:281 — CacheGetDir uses filepath.Walk to traverse the tree/ directory during integrity re-verification. filepath.Walk follows symlinks by default. An attacker with write access to the cache directory could plant a symlink pointing outside the cache root. Practical exploitability is low due to multiple defense layers (validateCachePath, atomicWrite, CachePutDir path traversal check).

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Intentional design: if there is no SKILL.md, there are no dependencies to declare. Validation of skill directory contents belongs at a different layer.

• [edge-case] internal/forge/github/github.go:880 — In listDirContents, when path is empty (repository root), the relative path computation uses relPath = e.Path. Behavior depends on the GitHub API returning paths without leading slashes, which is a stable API contract.

• [missing-migration-path] docs/ADRs/0038-universal-harness-access.md — The ADR updates skills from single files to directories but does not document a migration path for existing harnesses with single-file skill URLs. The code now rejects non-forge skill URLs at validation time with a clear error, and lock file backward compatibility is handled via empty Type defaulting to "file".

• [scope-expansion] PR title suggests "model skills as directories" but also implements lock file functionality (Phase 3). However, the lock file changes are tightly coupled to the directory model (Type/Files fields) and cannot easily be separated.

• [test-adequacy] internal/cli/lock_test.go — No test verifying the round-trip: generating a lock file with a directory skill via runLock and then using resolveFromLock to resolve from that lock file in a single test. Separate tests exist for each direction.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Essentially impossible in practice since URLs are programmatically constructed.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [stale-harness-example] docs/ADRs/0024-harness-definitions.md:233 — The harness schema example shows skills as individual SKILL.md files rather than directories, predating the directory model introduced by this PR.

Info

• [missing-authorization] No linked issue for a non-trivial 22-file PR. The change aligns with accepted ADR-0038, providing architectural rationale.

• [stale-reference-to-skill-format] docs/runtimes.md:12 — The security matrix references SKILL.md as a scanned artifact. This is not misleading — SKILL.md is still the file within a skill directory that gets scanned.

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go — ListDirectoryContents recurses into subdirectories with maxDirDepth=10 per-invocation, but a wide directory tree can trigger excessive GitHub API calls. Each subdirectory triggers a separate API call, and the resolver's maxResources limit counts resolved skills, not API calls within a single ListDirectoryContents invocation. A repo with 100 subdirs each containing 100 files at depth 1 would produce 101 API calls with no counter.
  Remediation: Add a cumulative API call counter or total file count limit to ListDirectoryContents to bound work within a single skill directory fetch.

• [fail-open] internal/resolve/resolve.go — In resolveSkillDirURL, the ForgeClient nil check happens before the CacheGetDir call. If ForgeClient is nil but the skill directory is already cached, the function errors out unnecessarily with "ForgeClient is required" instead of returning the cached result. This means the --offline flag path still requires a GitHub token even when the cache is fully populated.
  Remediation: Move the ForgeClient nil check to after the CacheGetDir call, inside the if !cacheHit block, so a cache hit doesn't require a ForgeClient.

• [edge-case] internal/forge/github/github.go — In ListDirectoryContents, the relative path computation strings.TrimPrefix(e.Path, path+"/") produces incorrect results when path is empty (repository root). ParseForgeURL allows empty Path (e.g., https://github.com/org/repo/tree/abc123), so this code path is reachable.
  Remediation: Guard the relPath computation: if path is empty, use e.Path directly as relPath. Similarly, in resolveSkillDirURL, handle empty forgeInfo.Path when constructing fullPath.

Low

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality: directory-aware skill resolution via forge API, directory cache with tree hashes, Type/Files fields in lock entries, ForgeURLInfo parser, and new files (url.go, url_test.go). Per COMMITS.md, fix is for user-visible bug fixes. This is feat(resolve): — GoReleaser uses the prefix to categorize release notes.

• [missing-authorization] No linked issue for a non-trivial 21-file PR. Filing an issue would document the rationale for this architectural change and provide explicit authorization.

• [portability] internal/fetch/cache.go — CacheGetDir uses filepath.Walk and filepath.Rel which produce backslash-separated paths on Windows, while CachePutDir stores files with forward-slash keys from the forge API. ComputeTreeHash would produce different results on Windows, causing false integrity verification failures.

• [consumer-completeness] internal/cli/lock.go:244 — resolveFromLock correctly defaults empty Type to "file" for backward compatibility, but there is no explicit test covering the scenario of loading a pre-existing lock file without Type/Files fields.

• [test-adequacy] internal/cli/lock_test.go — TestRunLock_SkillDirectoryType tests the lock-generation flow but does not verify that resolveFromLock correctly handles loading a directory-type dependency from the lock file (the reverse path).

• [architectural-coupling] internal/cli/lock.go, internal/cli/run.go — Both files directly instantiate gh.New(token) when HasURLSkills() is true. This follows the pre-existing pattern used by 13+ other call sites across the codebase, but adding a forge client factory would improve forge-agnosticism.

• [interface-expansion] internal/forge/forge.go — forge.Client interface expanded with ListDirectoryContents and GetFileContentAtRef. The interface already has 40+ methods; both LiveClient and FakeClient are updated. Future forge implementations (GitLab, Forgejo) will need to implement these.

• [adr-modification-without-annotation] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being modified to add the skill directory model. The modifications are additive clarifications, not contradictions, but a brief annotation explaining the update would preserve ADR hygiene.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file" which predates the directory model introduced by this PR.

• [path-traversal] internal/fetch/cache.go:281 — CachePutDir has an explicit path traversal check (strings.HasPrefix on cleaned paths) but does not check for symlinks within the tree directory after files are written. Unexploitable in practice since files come from forge API content bytes, not filesystem operations.

• [integrity-verification] internal/fetch/cache.go:332 — CacheGetDir's filepath.Walk follows symlinks by default. A planted symlink pointing outside the cache could be read into hash computation. Risk limited to information disclosure; integrity verification would fail, preventing use of tampered content.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compatibility for pre-directory-model lock files.

• [fail-open] internal/resolve/resolve.go — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Treats the skill as a leaf node — reasonable default since SKILL.md is the only file that declares transitive deps.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Very narrow edge case.

Review

Findings

Medium

• [unbounded-recursion] internal/forge/github/github.go — ListDirectoryContents recurses into subdirectories with no depth limit when recursive=true. A malicious repository (within allowed_remote_resources) with deeply nested directory structures could cause excessive API calls or stack overflow. The resolver's maxResources limit counts resolved skills, not API calls within a single ListDirectoryContents invocation.
  Remediation: Add a maxDepth parameter or internal depth counter (e.g., limit 10) to ListDirectoryContents.

• [scope-tier-mismatch] PR title uses fix(resolve): prefix but introduces substantial new functionality: directory cache (CachePutDir, CacheGetDir, ComputeTreeHash), forge API extensions (ListDirectoryContents, GetFileContentAtRef), ForgeURLInfo parser, Type/Files fields in lock entries, and new files (url.go, url_test.go, phase3.md, phase4.md). Per COMMITS.md, fix is for user-visible bug fixes. This is feat(resolve): — GoReleaser uses the prefix to categorize release notes.
  Remediation: Change commit prefix to feat(resolve): to accurately reflect that this adds new capability.

• [architectural-coupling] internal/cli/lock.go, internal/cli/run.go — Both files directly instantiate gh.New(token) when HasURLSkills() is true, hardcoding GitHub as the only forge. Per AGENTS.md, forge-specific logic belongs in internal/forge/. Adding GitLab or Forgejo support would require modifying CLI code.
  Remediation: Introduce a forge client factory that selects the appropriate client based on URL hostname or environment configuration.

• [interface-expansion] internal/forge/forge.go:168 — forge.Client interface expanded with two new methods: ListDirectoryContents and GetFileContentAtRef. The package is internal/forge/ (not externally importable in Go) and both LiveClient and FakeClient are already updated, so no code is currently broken. However, future forge implementations (GitLab, Forgejo) will need to implement these methods.
  Remediation: Document the new interface requirements. Consider interface segregation (e.g., forge.DirectoryClient) for forge implementations that don't support directory listing.

Low

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being modified to add the skill directory model throughout its Decision section. The modifications are additive (extending the existing design) rather than contradictory, but a brief annotation explaining the update would preserve ADR hygiene.

• [url-parsing] internal/forge/url.go — ParseForgeURL accepts both tree (directory) and blob (file) URL formats, but skills always expect tree. A blob URL would parse successfully but fail confusingly at ListDirectoryContents.

• [design-consistency] docs/plans/universal-harness-access-phase1.md — Phase 1 plan states "Validates that skill URLs are from supported forges (GitHub, GitLab)" but IsSupportedForge only accepts github.com. GitLab is claimed but not implemented.

• [breaking-change-undocumented] internal/lock/lock.go — DependencyEntry gains Type and Files fields but lock file version stays at 1. Backward compatibility is correctly handled via omitempty + default-to-file, but this schema evolution is undocumented.

• [consumer-completeness] internal/cli/lock.go:244 — resolveFromLock correctly defaults empty Type to "file" for backward compatibility, but there is no explicit test covering the scenario of loading a pre-existing lock file without Type/Files fields.

• [edge-case] internal/forge/url.go:34 — ParseForgeURL uses strings.LastIndex(rawURL, "#") to strip fragments before url.Parse. A literal # in a URL path (malformed per RFC 3986) would be incorrectly truncated. Very narrow edge case.

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, empty Type field defaults to "file". Intentional backward-compat for pre-directory-model lock files.

• [path-traversal] internal/fetch/cache.go:281 — CachePutDir has an explicit path traversal check (strings.HasPrefix) but does not check for symlinks within the tree directory after files are written. Files come from forge API (content bytes, not filesystem operations) so this is unexploitable in practice.

• [integrity-verification] internal/fetch/cache.go:332 — CacheGetDir's filepath.Walk follows symlinks by default. A planted symlink pointing outside the cache could be read into hash computation. Risk limited to information disclosure during hash computation; integrity verification would fail, preventing use of tampered content.

• [fail-open] internal/resolve/resolve.go:393 — resolveSkillTransitiveDeps returns nil (no error) if SKILL.md doesn't exist in the cached skill directory. Treats the skill as a leaf node — reasonable default since SKILL.md is the only file that declares transitive deps.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file" which predates the directory model introduced by this PR.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows skills as individual SKILL.md files rather than directories.

• [stale-skill-model-reference] docs/guides/user/customizing-agents.md — Multiple examples reference skills as .md files rather than directory structures.

Review

Findings

Medium

• [behavioral-inconsistency] internal/cli/run.go:222 — run.go silently proceeds with a nil ForgeClient when no GH_TOKEN/GITHUB_TOKEN is set (if token != "" { forgeClient = gh.New(token) } with no else clause), unlike lock.go which returns a hard error with a clear message ("Skill URLs require a GitHub token (set GH_TOKEN or GITHUB_TOKEN)"). When a harness has URL-referenced skills but no token is available, users get a confusing "ForgeClient is required to resolve skill URL" error from deep in resolveSkillDirURL instead of an early diagnostic at the CLI level.
  Remediation: Mirror lock.go's behavior in run.go: when HasURLSkills() is true and no token is found, emit printer.StepFail and return early with a clear message about setting GH_TOKEN or GITHUB_TOKEN.

• [robustness] internal/cli/lock.go:178 — When building lock entries for directory dependencies, CacheGetDir could return a nil dirEntry (e.g., if metadata is missing). The code guards with if dirEntry != nil but silently produces a lock entry with an empty Files manifest, losing per-file integrity metadata that consumers (resolveFromLock, future audit tools) expect for directory-type entries.
  Remediation: Return an error or log a warning when dirEntry is nil for a directory-type dependency. A directory lock entry without a file manifest is not useful for integrity verification.

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being substantially modified to add the skill directory model, forge interface extension, and directory cache. Per AGENTS.md, Accepted ADRs are point-in-time records. The modifications are additive (extending the design to handle directory-based skills), but the process guidance suggests creating a superseding ADR for substantial changes.
  Remediation: Add a clear annotation to ADR-0038 explaining the update, or create ADR-0039 that supersedes it with the directory-based skill model.

Low

• [fail-open] internal/cli/lock.go:270 — In resolveFromLock, an empty Type field in a lock entry defaults to "file". Intentional backward-compatibility measure for pre-directory-model lock files, but could cause unexpected behavior with corrupted or future-versioned lock files.

• [path-traversal] internal/fetch/cache.go:281 — CachePutDir has a path traversal check (strings.HasPrefix) but does not check for symlinks within the tree directory itself. Practical risk is low: validateCachePath covers the parent, cache is keyed by tree hash (requiring content prediction), and MkdirAll creates directories fresh.

• [scope-tier-mismatch] PR uses fix commit prefix but introduces substantial new functionality (directory-based skill model, forge API requirements, interface expansion). feat would be more accurate for GoReleaser release note categorization.

• [api-design] internal/forge/github/github.go — ListDirectoryContents recursively calls itself for subdirectories without a depth limit. Bounded indirectly by the resolver's MaxResources limit and GitHub API rate limiting.

• [url-parsing] internal/forge/url.go — ParseForgeURL accepts both tree (directory) and blob (file) URL formats, but skills always expect tree. A blob URL would pass validation but fail confusingly at ListDirectoryContents.

• [test-coverage] internal/cli/lock_test.go — TestRunLock_GeneratesLockFile no longer tests skills. A new TestRunLock_SkillDirectoryType provides mixed-type coverage (file agent + directory skill).

• [documentation] docs/plans/universal-harness-access.md — Plan doc code example for CachePutDir uses CacheEntry (single-file metadata struct) while actual implementation uses DirCacheEntry with Files field.

• [integrity-verification] internal/fetch/cache.go:332 — CacheGetDir's filepath.Walk follows symlinks by default; a symlink pointing outside the cache could be read into hash computation. Mitigated by validateCachePath on the parent dir and hash comparison.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file" which predates the directory model.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows skills as individual SKILL.md files rather than directories.

• [stale-skill-model-reference] docs/guides/user/customizing-agents.md — Multiple examples (lines 215, 227, 294) reference skills as .md files rather than directory structures.

• [stale-skill-model-reference] docs/agents/triage.md:60 — References creating skills as file paths without clarifying directory model.

Info

• [missing-authorization] No linked issue for a non-trivial 21-file PR. Filing an issue would document the rationale for this architectural change.

Review

Findings

Critical

• [error-handling-gap] internal/cli/lock.go:135 — runLock calls resolve.ResolveHarness without setting ForgeClient in ResolveOpts. The diff adds ForgeClient as a required field for skill directory resolution (resolveSkillDirURL checks opts.ForgeClient == nil and returns an error), but neither runLock nor runRun (run.go) are updated to provide a ForgeClient. Any harness with URL-referenced skills will fail at runtime with "ForgeClient is required to resolve skill URL". The lock_test.go was simultaneously changed to remove skill URL testing (replacing it with policy URL testing), masking this regression.
  Remediation: Pass a constructed forge.Client (e.g., github.NewLiveClient) as ForgeClient in the ResolveOpts at both lock.go and run.go. Restore or add a lock_test.go test that exercises the ForgeClient path.

Medium

• [test-integrity] internal/cli/lock_test.go:73 — TestRunLock_GeneratesLockFile was modified to replace skill URL testing with policy URL testing. No test in lock_test.go verifies that runLock correctly populates Type: "directory" and Files manifest for skill directory dependencies via the runLock integration path. The same pattern repeats in TestRunLock_AlreadyUpToDate and TestRunLock_UpdateForceReResolve. While resolve_test.go has comprehensive skill directory tests, the integration path through runLock is untested.
  Remediation: Add a lock_test.go test that uses forge.FakeClient to verify runLock resolves skill directory URLs, populates Type and Files in the lock entry, and passes ForgeClient through to ResolveHarness.

• [path-traversal] internal/forge/github/github.go — GetFileContentAtRef and ListDirectoryContents do not URL-path-escape the path parameter in the API URL. owner and repo use url.PathEscape but path does not. The code checks for ? and # characters but does not escape path segments. While the actual risk is mitigated by paths originating from ParseForgeURL (structured input) and downstream CachePutDir path traversal checks, the inconsistency with how owner and repo are handled is a defense-in-depth gap.
  Remediation: Escape each segment of path individually using url.PathEscape (split on /, escape each segment, rejoin), consistent with how owner and repo are handled.

• [adr-modification-justification] docs/ADRs/0038-universal-harness-access.md — ADR-0038 has status "Accepted" and is being substantially modified to add the skill directory model, forge interface extension, and directory cache. Per AGENTS.md: "Once an ADR on main has status Accepted, it is a point-in-time record. Do not substantially rewrite its Context, Decision, or Consequences sections."
  Remediation: Create ADR-0039 that supersedes ADR-0038 with the directory-based skill model, or add a "Status Note" section explaining why inline modification is appropriate here.

• [interface-expansion-breaking] internal/forge/forge.go:168 — forge.Client interface expanded with two new methods: ListDirectoryContents and GetFileContentAtRef. While this is an internal/ package (not externally importable), it affects all future forge implementations (GitLab, Forgejo) which will need to implement these methods.
  Remediation: Document the new interface requirements. Consider whether interface segregation would be cleaner for forge implementations that don't support directory listing.

Low

• [error-handling-gap] internal/cli/lock.go:155 — In runLock, when dep.Type == "directory", a CacheGetDir miss (nil dirEntry) silently produces a lock entry with no Files manifest. Since the directory was just fetched and cached moments before, a nil dirEntry indicates a logic bug.

• [fail-open] internal/cli/lock.go:238 — In resolveFromLock, when lockDep.Type is empty, it defaults to "file". Intentional backward compatibility for pre-directory-model lock files.

• [inconsistency] docs/plans/universal-harness-access.md:924 — Plan doc code example for CachePutDir uses CacheEntry (single-file metadata struct) while actual implementation uses DirCacheEntry with Files field.

• [path-injection] internal/forge/github/github.go — ListDirectoryContents uses TrimPrefix to compute relative paths. If API returns a path not matching the expected prefix, TrimPrefix returns the string unchanged. Defense-in-depth is present via CachePutDir's path traversal check.

• [design-coherence] internal/forge/url.go — ParseForgeURL accepts both tree (directory) and blob (file) URL formats, but skills always expect tree. A blob URL would parse successfully then fail at ListDirectoryContents with a potentially confusing error.

• [stale-skill-model-reference] docs/glossary.md:140 — Skill definition describes skills as "A markdown file (optionally with a scripts/ directory)" which predates the directory model.

• [stale-skill-model-reference] docs/ADRs/0024-harness-definitions.md:233 — Directory listing shows old SKILL.md format, doesn't reflect the directory tree model.

• [schema-evolution-backward-compatible-read] internal/lock/lock.go:38 — Lock file schema evolves without version bump. New Type/Files fields use omitempty, maintaining backward compatibility.

• [architecture-alignment] internal/harness/harness.go — ValidateResourceTypes enforces forge-only for skill URLs. The ADR documents this asymmetry but could be more prominent in the resource types table.

Info

• [rate-limiting] internal/forge/github/github.go — ListDirectoryContents recursively calls itself for subdirectories with no depth limit or max entry count. Bounded indirectly by the resolver's MaxResources limit.

• [missing-authorization] No linked issue for a non-trivial PR (22 files changed). Filing an issue would document the rationale.

• [scope-tier-mismatch] PR uses fix commit prefix but introduces substantial new functionality (forge API methods, directory caching, lock file schema extensions). feat would be more accurate for GoReleaser categorization.